perf(middleware/logger): optimize color status #3603
Conversation
exoego
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3603 +/- ##
==========================================
- Coverage 94.71% 94.71% -0.01%
==========================================
Files 158 158
Lines 9555 9554 -1
Branches 2813 2885 +72
==========================================
- Hits 9050 9049 -1
Misses 505 505 ☔ View full report in Codecov by Sentry. |
exoego
force-pushed
the
refactor-colorStatus
branch
from
a95bcce
to
0b7121a
Compare
exoego
changed the title
exoego
changed the title
| // Fallback to unsupported status code. | ||
| // E.g.) Bun and Deno supports new Response with 101, but Node.js does not. | ||
| // And those may evolve to accept more status. | ||
| return `${status}` |
There was a problem hiding this comment.
The line is covered for color-disabled case.
But not covered for currently unsupported status code, because test always fail if we test 100 or 700.
This fallback will help when Response supports 100 or other in future.
There was a problem hiding this comment.
Hi @exoego, thanks for creating the pull request.
It's a fine line as to whether we need to overwrite the object and test it, but it's better if there is test, so how about testing it with the following code?
diff --git a/src/middleware/logger/index.test.ts b/src/middleware/logger/index.test.ts
index 0623fc64..7c7e4689 100644
--- a/src/middleware/logger/index.test.ts
+++ b/src/middleware/logger/index.test.ts
@@ -32,7 +32,12 @@ describe('Logger by Middleware', () => {
return c.redirect('/empty', 301)
})
app.get('/server-error', (c) => {
- return c.text('', 511)
+ const res = new Response('', { status: 511 })
+ if (c.req.query('status')) {
+ // test status code not yet supported by runtime `Response` object
+ Object.defineProperty(res, 'status', { value: parseInt(c.req.query('status') as string) })
+ }
+ return res
})
})
@@ -95,6 +100,22 @@ describe('Logger by Middleware', () => {
expect(log.startsWith('--> GET /server-error \x1b[31m511\x1b[0m')).toBe(true)
expect(log).toMatch(/m?s$/)
})
+
+ it('Log status 100', async () => {
+ const res = await app.request('http://localhost/server-error?status=100')
+ expect(res).not.toBeNull()
+ expect(res.status).toBe(100)
+ expect(log.startsWith('--> GET /server-error 100')).toBe(true)
+ expect(log).toMatch(/m?s$/)
+ })
+
+ it('Log status 700', async () => {
+ const res = await app.request('http://localhost/server-error?status=700')
+ expect(res).not.toBeNull()
+ expect(res.status).toBe(700)
+ expect(log.startsWith('--> GET /server-error 700')).toBe(true)
+ expect(log).toMatch(/m?s$/)
+ })
})
describe('Logger by Middleware in NO_COLOR', () => {There was a problem hiding this comment.
Oh that's hacky but nice.
Done in 66f35bf
|
Thanks!
You are right. Good point! |
This PR optimizes
colorStatusfunction in Logger middleware.Performance-wise, both
if-elseversion andswitchversion is the best.I chose
switchversion which appears cleaner to me.I've noticed some status codes (e.g.
700) are unreachable, so deleted.Benchmark
282 times faster on Node:
1020 times faster on Bun:
479 times faster on Deno:
The author should do the following, if applicable
bun run format:fix && bun run lint:fixto format the code